Handle custom handlers in a more principled way#2194
Conversation
|
I'd personally rather have the normal boring code rather than another macro, it's hard for me to follow what's going with the handler inside the macro, and it's also confusing for navigation, recently someone couldn't figure out where a handler was being invoked, because it was called through one of those macros. But I won't block it if you think it's really better. |
|
I also prefer having the "normal boring code", but it doesn't allow handlers to be processed in parallel, nor does it to use the If we wanted the same thing as the macro but without the macro, it would look like this: let check_commits_fut = async {
if let Ok(config) = &config {
check_commits::handle(ctx, host, event, &config)
.await
.map_err(|e: anyhow::Error| {
HandlerError::Other(e.context(format!(
"error when processing check_commits handler",
)))
})
}
}
// for each custom handler do the same thing as above
let (check_commits_ret, ...) = futures::join!(check_commits_fut, ...);
if let Err(err) = check_commits_ret {
errors.push(err);
}That's a lot of boilerplate. Most of them are removed by the macro. |
let mut futs = vec![];
if let Ok(config) = &config {
futs.push(async { check_commits::handle(ctx, host, event, &config)
.await
.map_err(|e: anyhow::Error| {
HandlerError::Other(e.context(format!(
"error when processing check_commits handler",
)))
}) });
};
let errors = futures::future::join_all(futs).await.into_iter().filter_map(|res| res.err()).collect::<Vec<_>>();Doesn't seem that terrible. |
|
I just tried your alternative and it won't work because all the The snippet I proposed above would work as it doesn't need dyn-compatibity. If you're fine with it, I can change the PR to use explicit variable instead of a macro. |
|
The code was very pseudo-cody 😆 It would need I'm fine with your solution. |
Of-course and I tried with |
|
Isn't that just |
|
Indeed |
This PR modifies the handling of our "custom" handlers (the one not using
issue_handlers!orcommand_handlers!)to ain order for:custom_handlers!macroHandlerErrorinfra, instead of the custom loggingI also took the opportunity to move the macros to a dedicatedhandlers/macros.rsfile.Based on #2192 (only last 2 commits are different)